Add access modifier to `VolumeVO` by FelipeM525 · Pull Request #9394 · apache/cloudstack

FelipeM525 · 2024-07-15T11:04:51Z

Description

The class VolumeVO lacks access modifiers in its fields. This PR aims to improve adherence to object-oriented programming by adding private access modifiers to all fields in the class mentioned above, along with their respective getters and setters. I've also added a getter to an occurrence in VolumeApiServiceImpl where a field belonging to VolumeVO was being accessed directly.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

I ran all the tests related to VolumeVO and VolumeApiServiceImpl, checked all usages of both classes, and made sure nothing was broken.

codecov · 2024-07-15T13:52:07Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 15.81%. Comparing base (15e25bc) to head (67dbfe3).
Report is 139 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...hema/src/main/java/com/cloud/storage/VolumeVO.java	0.00%	2 Missing ⚠️
...n/java/com/cloud/storage/VolumeApiServiceImpl.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20    #9394      +/-   ##
============================================
- Coverage     15.81%   15.81%   -0.01%     
+ Complexity    12554    12553       -1     
============================================
  Files          5629     5629              
  Lines        492023   492023              
  Branches      62519    63929    +1410     
============================================
- Hits          77813    77812       -1     
  Misses       405887   405887              
- Partials       8323     8324       +1

Flag	Coverage Δ
uitests	`4.48% <ø> (ø)`
unittests	`16.60% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DaanHoogland · 2024-07-15T15:25:12Z

good code @FelipeM525, but is there a cause for this? I mean is there a plan to exploit this ?

weizhouapache · 2024-07-15T16:11:21Z

good code @FelipeM525, but is there a cause for this? I mean is there a plan to exploit this ?

I have same questions

FelipeM525 · 2024-07-16T13:23:03Z

good code @FelipeM525, but is there a cause for this? I mean is there a plan to exploit this ?

Hello @DaanHoogland, Given the scope of Cloudstack and the number of people that work on this project, I believe it's important to emphasize keeping the code base clean by applying clean code principles and respecting the main concept of Java, which is OOP. Therefore, we should make sure classes have private access modifiers and that fields aren't accessed directly without getters so as to adhere to encapsulation.

GaOrtiga

CLGTM

JoaoJandre

CLGTM

JoaoJandre · 2024-07-18T11:40:54Z

@blueorangutan package

blueorangutan · 2024-07-18T11:42:03Z

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-07-18T13:00:24Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10397

weizhouapache · 2024-07-18T13:11:07Z

@blueorangutan test

blueorangutan · 2024-07-18T13:12:03Z

@weizhouapache a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-07-18T13:39:07Z

[SF] Trillian Build Failed (tid-10909)

FelipeM525 · 2024-08-01T14:21:45Z

@weizhouapache could you run the test again?

weizhouapache · 2024-08-01T14:38:35Z

@blueorangutan test rocky8 kvm-rocky8

blueorangutan · 2024-08-01T14:40:03Z

@weizhouapache a [SL] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

blueorangutan · 2024-08-02T04:57:13Z

[SF] Trillian test result (tid-11008)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 49272 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9394-t11008-kvm-rocky8.zip
Smoke tests completed. 136 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_06_purge_expunged_vm_background_task	`Failure`	332.10	test_purge_expunged_vms.py

weizhouapache · 2024-08-02T08:08:25Z

@FelipeM525

hostip should be renamed to hostIp
getHostip and setHostip are not needed. there are already getHostIp and setHostIp

apache#9394 (comment)

weizhouapache

@blueorangutan package

JoaoJandre · 2024-08-15T13:05:42Z

@blueorangutan package

blueorangutan · 2024-08-15T13:08:04Z

@JoaoJandre a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-08-15T14:19:36Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10673

FelipeM525 · 2024-08-16T11:27:35Z

@weizhouapache could you run tests?

DaanHoogland · 2024-08-20T09:15:54Z

@blueorangutan LLtest

blueorangutan · 2024-08-20T09:18:03Z

@DaanHoogland a [LL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

github-actions · 2024-09-09T13:31:10Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

…ere a field was being accessed without a getter

blueorangutan · 2024-09-11T13:51:52Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11097

JoaoJandre · 2024-10-04T19:49:43Z

for the debian build I see

10:02:05 [ERROR] Failed to execute goal on project cloud-server: Could not resolve dependencies for project org.apache.cloudstack:cloud-server:jar:4.20.0.0-shapeblue11941: Could not transfer artifact org.slf4j:slf4j-api:jar:1.7.32 from/to central (https://repo.maven.apache.org/maven2): transfer failed for https://repo.maven.apache.org/maven2/org/slf4j/slf4j-api/1.7.32/slf4j-api-1.7.32.jar: Connection reset -> [Help 1]

I don't think we still have a slf4j dependency as it was removed and logging was consolidated to be all log4j2???

The log4j 1.2 dependency could not be completely removed because juniper contrail has log4j as a dependency. So we still have this dependency somewhere in the code.

sureshanaparti

clgtm

sureshanaparti · 2024-10-07T12:11:15Z

@blueorangutan package

blueorangutan · 2024-10-07T12:12:03Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-10-07T13:11:02Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11295

BryanMLima

CLGTM

BryanMLima · 2024-11-29T13:59:02Z

@blueorangutan package

blueorangutan · 2024-11-29T14:00:05Z

@BryanMLima a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-11-29T15:06:22Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11659

JoaoJandre · 2024-12-02T19:22:46Z

@DaanHoogland could we run the CI here?

DaanHoogland · 2024-12-04T08:13:36Z

@DaanHoogland could we run the CI here?

@JoaoJandre , our lab has been very busy the last few days. I'll have a go later tonight or in the weekend.

DaanHoogland · 2024-12-04T16:35:36Z

@blueorangutan test

blueorangutan · 2024-12-04T16:36:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

DaanHoogland · 2024-12-05T10:33:16Z

@FelipeM525 @JoaoJandre , I did not investigate deeply but systemVMs do not start in this env:

2024-12-05 10:28:41,870 ERROR [c.c.v.VmWorkJobDispatcher] (Work-Job-Executor-79:[ctx-d0456350, job-6/job-392]) logid:c2de6f47) Unable to complete AsyncJobVO: {id:392, userId: 1, accountId: 1, instanceType: null, instanceId: null, cmd: com.cloud.vm.VmWorkStart, cmdInfo: rO0ABXNyABhjb20uY2xvdWQudm0uVm1Xb3JrU3RhcnR9cMGsvxz73gIAC0oABGRjSWRMAAZhdm9pZHN0ADBMY29tL2Nsb3VkL2RlcGxveS9EZXBsb3ltZW50UGxhbm5lciRFeGNsdWRlTGlzdDtMAAljbHVzdGVySWR0ABBMamF2YS9sYW5nL0xvbmc7TAAGaG9zdElkcQB-AAJMAAtqb3VybmFsTmFtZXQAEkxqYXZhL2xhbmcvU3RyaW5nO0wAEXBoeXNpY2FsTmV0d29ya0lkcQB-AAJMAAdwbGFubmVycQB-AANMAAVwb2RJZHEAfgACTAAGcG9vbElkcQB-AAJMAAlyYXdQYXJhbXN0AA9MamF2YS91dGlsL01hcDtMAA1yZXNlcnZhdGlvbklkcQB-AAN4cgATY29tLmNsb3VkLnZtLlZtV29ya5-ZtlbwJWdrAgAESgAJYWNjb3VudElkSgAGdXNlcklkSgAEdm1JZEwAC2hhbmRsZXJOYW1lcQB-AAN4cAAAAAAAAAABAAAAAAAAAAEAAAAAAAAAd3QAGVZpcnR1YWxNYWNoaW5lTWFuYWdlckltcGwAAAAAAAAAAHBwcHBwcHBwcHA, cmdVersion: 0, status: IN_PROGRESS, processStatus: 0, resultCode: 0, result: null, initMsid: 32986238026845, completeMsid: null, lastUpdated: null, lastPolled: null, created: Thu Dec 05 10:28:33 UTC 2024, removed: null}, job origin: 6 com.cloud.exception.InsufficientServerCapacityException: Unable to create a deployment for VM instance {"id":119,"instanceName":"s-119-VM","type":"SecondaryStorageVm","uuid":"7dd52cd9-eea0-419a-90f0-1fdc74f3d2c0"}Scope=interface com.cloud.dc.DataCenter; id=1
        at com.cloud.vm.VirtualMachineManagerImpl.orchestrateStart(VirtualMachineManagerImpl.java:1237)
        at com.cloud.vm.VirtualMachineManagerImpl.orchestrateStart(VirtualMachineManagerImpl.java:5467)
        at jdk.internal.reflect.GeneratedMethodAccessor290.invoke(Unknown Source)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:569)
        at com.cloud.vm.VmWorkJobHandlerProxy.handleVmWorkJob(VmWorkJobHandlerProxy.java:106)
        at com.cloud.vm.VirtualMachineManagerImpl.handleVmWorkJob(VirtualMachineManagerImpl.java:5591)
        at com.cloud.vm.VmWorkJobDispatcher.runJob(VmWorkJobDispatcher.java:99)
        at org.apache.cloudstack.framework.jobs.impl.AsyncJobManagerImpl$5.runInContext(AsyncJobManagerImpl.java:652)
        at org.apache.cloudstack.managed.context.ManagedContextRunnable$1.run(ManagedContextRunnable.java:49)
        at org.apache.cloudstack.managed.context.impl.DefaultManagedContext$1.call(DefaultManagedContext.java:56)
        at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.callWithContext(DefaultManagedContext.java:103)
        at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.runWithContext(DefaultManagedContext.java:53)
        at org.apache.cloudstack.managed.context.ManagedContextRunnable.run(ManagedContextRunnable.java:46)
        at org.apache.cloudstack.framework.jobs.impl.AsyncJobManagerImpl$5.run(AsyncJobManagerImpl.java:600)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:840)

DaanHoogland · 2024-12-05T11:15:17Z

@blueorangutan package

blueorangutan · 2024-12-05T11:16:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-12-05T11:26:35Z

[SF] Trillian Build Failed (tid-11856)

blueorangutan · 2024-12-05T12:33:50Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11724

DaanHoogland · 2024-12-05T14:20:03Z

@blueorangutan test

blueorangutan · 2024-12-05T14:22:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2024-12-06T06:37:11Z

[SF] Trillian test result (tid-11859)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 56110 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9394-t11859-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

* added private access modifiers to VolumeVO and fixed an occurrence where a field was being accessed without a getter * renamed the field hostip to hostIp and removed duplicated methods apache#9394 (comment)

Port 4.20 - Add access modifier to VolumeVO apache#9394 See merge request scclouds/scclouds!1057

boring-cyborg bot added the component:storage label Jul 15, 2024

GaOrtiga approved these changes Jul 17, 2024

View reviewed changes

JoaoJandre approved these changes Jul 18, 2024

View reviewed changes

weizhouapache approved these changes Jul 18, 2024

View reviewed changes

FelipeM525 added a commit to scclouds/cloudstack that referenced this pull request Aug 2, 2024

renamed the field hostip to hostIp and removed duplicated methods

2a376f3

apache#9394 (comment)

weizhouapache approved these changes Aug 2, 2024

View reviewed changes

github-actions bot added the status:has-conflicts label Sep 9, 2024

added private access modifiers to VolumeVO and fixed an occurrence wh…

a556657

…ere a field was being accessed without a getter

DaanHoogland added this to the 4.20.1.0 milestone Sep 16, 2024

sureshanaparti approved these changes Oct 7, 2024

View reviewed changes

BryanMLima approved these changes Nov 29, 2024

View reviewed changes

DaanHoogland changed the base branch from main to 4.20 December 2, 2024 08:51

weizhouapache merged commit 810c410 into apache:4.20 Dec 6, 2024

nicoschmdt pushed a commit to scclouds/cloudstack that referenced this pull request Feb 3, 2025

Port 4.20 - Add access modifier to VolumeVO apache#9394

e5da950

nicoschmdt pushed a commit to scclouds/cloudstack that referenced this pull request Feb 3, 2025

Merge branch 'BackPort-PR9394-to-4.20.0.1' into '4.20.0.0-scclouds'

1ee7668

Port 4.20 - Add access modifier to VolumeVO apache#9394 See merge request scclouds/scclouds!1057

Pearl1594 added this to ACS 4.20.1 Mar 17, 2025

Pearl1594 moved this to Done in ACS 4.20.1 Mar 17, 2025

Conversation

FelipeM525 commented Jul 15, 2024

Description

Types of changes

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

codecov bot commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Jul 15, 2024

Uh oh!

weizhouapache commented Jul 15, 2024

Uh oh!

FelipeM525 commented Jul 16, 2024

Uh oh!

GaOrtiga left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoJandre commented Jul 18, 2024

Uh oh!

blueorangutan commented Jul 18, 2024

Uh oh!

blueorangutan commented Jul 18, 2024

Uh oh!

weizhouapache commented Jul 18, 2024

Uh oh!

blueorangutan commented Jul 18, 2024

Uh oh!

blueorangutan commented Jul 18, 2024

Uh oh!

FelipeM525 commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weizhouapache commented Aug 1, 2024

Uh oh!

blueorangutan commented Aug 1, 2024

Uh oh!

blueorangutan commented Aug 2, 2024

Uh oh!

weizhouapache commented Aug 2, 2024

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

JoaoJandre commented Aug 15, 2024

Uh oh!

blueorangutan commented Aug 15, 2024

Uh oh!

blueorangutan commented Aug 15, 2024

Uh oh!

FelipeM525 commented Aug 16, 2024

Uh oh!

DaanHoogland commented Aug 20, 2024

Uh oh!

blueorangutan commented Aug 20, 2024

Uh oh!

github-actions bot commented Sep 9, 2024

Uh oh!

blueorangutan commented Sep 11, 2024

Uh oh!

JoaoJandre commented Oct 4, 2024

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Oct 7, 2024

Uh oh!

blueorangutan commented Oct 7, 2024

Uh oh!

blueorangutan commented Oct 7, 2024

Uh oh!

BryanMLima left a comment

Choose a reason for hiding this comment

Uh oh!

BryanMLima commented Nov 29, 2024

codecov bot commented Jul 15, 2024 •

edited

Loading

FelipeM525 commented Aug 1, 2024 •

edited

Loading